[Yoga Beta] Improvements to the experimental support for Yoga layout. - #59
Conversation
Yoga remains an unsupported / speculative feature, but these improvements are important for the functionality of clients that are experimenting with it. For example, without these changes, ASButtonNode is not able to lay out correctly. These changes allow certain subtrees that use layout specs to coexist properly in a Yoga heirarchy. The most significant change here is moving ASEdgeInsets into the #if YOGA gating. Although this is technically an API change, this type was added with no known use cases and is really only useful for flexbox layout specification. So, before usages of it are created, it makes sense to constrain the Texture API surface until that time.
|
|
…ent RTL support. Although apps could handle this before by setting the view's property in didLoad, it's useful to bridge this property for setting during off-main initialization. This change also makes RTL fully functional / automatic for Yoga layout users.
e50b2e3 to
5cdf50d
Compare
|
This has now merged latest master and should be ready to land, including RTL support as provided by PR #60. |
|
🚫 CI failed with log |
Adlai-Holler
left a comment
Adlai-Holler
left a comment
There was a problem hiding this comment.
Very good stuff here! A lot more refined and complete implementation than before, and I agree with the decision to limit the surface area and gate ASEdgeInsets on yoga until it's needed elsewhere.
Comments are non-blocking.
| #if YOGA | ||
| std::atomic<ASStackLayoutDirection> _direction; | ||
| std::atomic<ASStackLayoutDirection> _flexDirection; | ||
| std::atomic<YGDirection> _direction; |
There was a problem hiding this comment.
(You knew it was coming!) Maybe these would be better-off as Objective-C atomic properties to improve performance and cut down on boilerplate?
There was a problem hiding this comment.
As all of them are currently c++ atomics let's go with it for now and do the whole change in a another diff if we want to.
| [UIView userInterfaceLayoutDirectionForSemanticContentAttribute:attribute]; | ||
| self.style.direction = (layoutDirection == UIUserInterfaceLayoutDirectionLeftToRight | ||
| ? YGDirectionLTR : YGDirectionRTL); | ||
| } |
There was a problem hiding this comment.
Nice! That class method may not be safe off-main, but I expect you've considered that. Even if so, the risk here is teeny tiny.
|
|
||
| @property (nonatomic, assign, readwrite) ASStackLayoutDirection direction; | ||
| @property (nonatomic, assign, readwrite) ASStackLayoutDirection flexDirection; | ||
| @property (nonatomic, assign, readwrite) YGDirection direction; |
There was a problem hiding this comment.
Even if these are implemented using C++ atomics, we ought to declare them as atomic.
|
As I'm currently working in this area and this PR can introduce some merge conflicts, I rather would prevent this and I will try to get this PR further and hopefully in sooner than later cc @appleguy @Adlai-Holler |
|
🚫 CI failed with log |
|
🚫 CI failed with log |
|
🚫 CI failed with log |
|
@maicki @Adlai-Holler thanks for your comments here and for merging this - sorry for the slow response on my side. I could put up another change that includes atomic, too, but won't worry about it until the other changes @maicki mentioned are landed. One more thing - @maicki it's totally OK if your refactor removes the Yoga hook from ASDisplayNode entirely. Just drop me a message if you do this and I will work on re-adding it in the least invasive way possible. I actually had some recent ideas about how to possibly use the ASLayoutSpec API to build the Yoga tree; it would be a bit unusual because it would have to connect with the Yoga tree across levels of the hierarchy, BUT the integration points would basically not touch ASDisplayNode. |
5 participants
Yoga remains an unsupported / speculative feature, but these improvements are important for
the functionality of clients that are experimenting with it.
Total list of changes:
For example, without these changes, ASButtonNode is not able to lay out correctly. These
changes allow certain subtrees that use layout specs to coexist properly in a Yoga heirarchy.
The most significant change here is moving ASEdgeInsets into the #if YOGA gating. Although
this is technically an API change, this type was added with no known use cases and is
really only useful for flexbox layout specification. So, before usages of it are created,
it makes sense to constrain the Texture API surface until that time.